Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance Metadata.md to address issue #78 #80

Merged
merged 4 commits into from
May 16, 2022
Merged

Enhance Metadata.md to address issue #78 #80

merged 4 commits into from
May 16, 2022

Conversation

Olf0
Copy link
Collaborator

@Olf0 Olf0 commented Apr 10, 2022

No description provided.

… because the mix is confusing; one may incorrectly suppose a reason.
@Olf0 Olf0 marked this pull request as draft April 10, 2022 22:47
@Olf0
Copy link
Collaborator Author

Olf0 commented Apr 10, 2022

@rinigus,

  1. Is this really correct? I added the sentence, "If Repo: is not set or the metadata for SailfishOS:Chum is completely missing, the URL provided by the URL: field in the spec file preamble is used instead."
    You wrote that the "RPM SPEC URL [is] used as a fallback for Url/Repo", but AFAICS in chumpackage.cpp m_url is preset with the URL: field of the spec file preamble (in line 118) and then used as the default value for the Homepage: field (in line 210). But the m_repo_url is left empty when not set (see line 203) before entering projectgithub.cpp or projectgitlab.cpp; what these two do is hard to grasp for me as a former Shell and C programmer.
  2. For the field DeveloperName: the comment states If not set, and a GitHub repository is set, then the name will be automatically retrieved. Note that such automatic retrieval is not supported for GitLab repositories.
    • a. The statement "a GitHub repository is set" is imprecise: By which field(s)? Presumably as Repo:-URL, but with or without the fallback mechanism discussed in 1., above?
    • b. This automatic retrieval of the name is only described for the field DeveloperName:, but not for the field PackagerName:. I think they should be handled then same, i.e., If not set, and a GitHub repository is set as PackagingRepo:, then the name will be automatically retrieved. Note that such automatic retrieval is not supported for GitLab repositories. Is this already the case?
    • c. For obtaining a name at GitLab, as a fallback the URL might be parsed, though this provides the login name, not the pretty name, see, e.g.: https://gitlab.com/Olf0/sailfishX Although the pretty name can be extracted as the first string on a GitLab user page proper (when ignoring GitLab's top menu bar), see, e.g.: https://gitlab.com/Olf0

@rinigus
Copy link
Collaborator

rinigus commented Apr 11, 2022

@Olf0: I'll reply few days later, sorry. Will check the code as well

@rinigus
Copy link
Collaborator

rinigus commented Apr 12, 2022

@Olf0:

1: yes, it is correct. Loop at https://github.com/sailfishos-chum/sailfishos-chum-gui/blob/main/src/chumpackage.cpp#L215 runs through {m_repo_url, m_url, m_packaging_repo_url} and if m_repo_url is not set it will try m_url next. That URL is used in constructor for GitHub/GitLab projects (if they fit the scheme as tested by ::isProject) and through that would support corresponding features (issues, releases)

2a,b: If any of the three is set ({m_repo_url, m_url, m_packaging_repo_url}), developer name would be set from that if needed. Now, indeed, packager name is not currently set if we query m_packaging_repo_url. Surely can be done, just wasn't as a part of the spec when I implemented it. Can be considered as a bug / missing feature

2c: We can consider doing it, indeed.

@Olf0
Copy link
Collaborator Author

Olf0 commented Apr 14, 2022

These were quick "few days" (just one)! 😉

1: yes, it is correct.

Thank you for the clarification.

2a: If any of the three is set ({m_repo_url, m_url, m_packaging_repo_url}), developer name would be set from that if needed.

For me it seems to be not O.K. to extract the developer's name from the m_packaging_repo_url, even as a second fallback.

2b: Now, indeed, packager name is not currently set if we query m_packaging_repo_url.

In contrast to the behaviour you described for 2a, this appears to be logical.

Surely can be done, just wasn't as a part of the spec when I implemented it. Can be considered as a bug / missing feature.

O.K. Which of the following two options do you prefer?

  1. Leave everything as it is (code and this PR), and mark this PR as "ready for review"?
  2. Address 2a & 2b in the code, while I extend this PR to cover this behaviour.

2c: We can consider doing it, indeed.

In general I do not like web-page "scraping", because web-pages are often altered. But in this case, I believe that the pretty name will very likely always be right below the avatar picture, so this is a sustainable solution.

Filed as issue #81.

@Olf0
Copy link
Collaborator Author

Olf0 commented Apr 19, 2022

@rinigus, before we get side-tracked into point 2c / issue #81 even more, the main open question for this PR is (WRT points 2a & 2b):

O.K. Which of the following two options do you prefer?

  1. Leave everything as it is (code and this PR), and mark this PR as "ready for review"?
  2. Address 2a & 2b in the code, while I extend this PR to cover this behaviour.

@rinigus
Copy link
Collaborator

rinigus commented Apr 19, 2022

@Olf0: sorry for delays. I think it would make sense to proceed with as it is. In general, with Chum, we would have to work taking into account that Jolla can pull the plug on OBS in May. So, before we invest a lot of time into improvement of it, would be nice to get OBS status reaffirmed. On the other side, we shouldn't let it rot either as that would be a mistake as well. Would have to balance those aspects :)

@Olf0
Copy link
Collaborator Author

Olf0 commented May 13, 2022

@Olf0: sorry for delays. I think it would make sense to proceed with as it is.

O.K., done (sorry I had forgotten to do this).

In general, with Chum, we would have to work taking into account that Jolla can pull the plug on OBS in May.

Oh, I missed that. Where does that May deadline come from? I had the impression that (after the heated discussion at FSO) the Sailfish-OBS will be maintained until Jolla provides a viable alternative, which I expect to come in "Jolla time": years or never.

@Olf0 Olf0 marked this pull request as ready for review May 13, 2022 00:59
@Olf0
Copy link
Collaborator Author

Olf0 commented May 14, 2022

I found it. Thank you for posing the question to the IRC community meeting 2022-05-12.

Oh, I missed that. Where does that May deadline come from? I had the impression that (after the heated discussion at FSO) the Sailfish-OBS will be maintained until Jolla provides a viable alternative, which I expect to come in "Jolla time": years or never.

As expected, the answer is vague and tardy, but the undertone appears to be negative. 😞

@rinigus
Copy link
Collaborator

rinigus commented May 15, 2022

Indeed, no clear reply and we have to search for undertones without being given +/- and their contributions to decision.

@Olf0
Copy link
Collaborator Author

Olf0 commented May 15, 2022

Still …

I think it would make sense to proceed with as it is.

Or is there anything holding you back from merging this (and also PR #79)?

@rinigus
Copy link
Collaborator

rinigus commented May 16, 2022

@Olf0: sorry, just forgot to do so.

@rinigus rinigus merged commit c1269bb into sailfishos-chum:main May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants